Conversation
Document the ava-sdk-js TypeScript enum update, backward compat handling for legacy proto value 4, and test migration scope.
extractResultData in the execution queue was missing the GraphQL output case. GraphQL HTTP requests executed successfully and steps were marked Success=true, but the result data was never extracted — returning nil. The loop aggregation then counted all nil results as failed iterations, producing "2 of 2 iterations failed" even though the queries succeeded. Add the missing step.GetGraphql() case to extractResultData and to the debug logging switch.
|
Code Review — PR #524 Overall this is a clean, targeted fix. The root cause is clear, the change is minimal, and the regression test proves the fix. A few observations below. vm.go — extractResultData fix (core/taskengine/vm.go:4258) Correct and well-patterned. Adding the GraphQL case mirrors how every other output type is handled. The double nil-guard (graphqlOutput != nil and graphqlOutput.Data != nil) follows the same pattern used for customCode and restApi above it. No metadata wrapping is needed here since GraphQL queries are read-only with no tx hash to propagate. vm.go — logging switch (core/taskengine/vm.go:4191) Fine addition, but the switch is already missing EthTransfer (also handled in extractResultData). The logging node-type will show "unknown" for ETH transfer iterations. Not introduced by this PR, but worth a follow-up to keep the switch in sync with extractResultData. Test — unused body decode (vm_runner_graphql_query_test.go:183-184) The mock handler decodes the request body into a Test — result content not validated The test confirms each iteration result is non-nil (exactly what the bug fix requires), but does not assert the result actually contains the expected Documentation (docs/changes/0001-execution-status-redesign.md) Clear and well-structured. The backward-compatibility mapping for legacy PARTIAL_SUCCESS (proto value 4) to Failed is explicitly called out, which is exactly what consumers need when upgrading. Good to have this in the design doc rather than just the SDK PR. Summary:
No blocking issues. The two nits (unused body decode, missing EthTransfer in logging switch) are small enough to address in a follow-up. |
There was a problem hiding this comment.
Pull request overview
Fixes Loop node behavior when using the GraphQL runner by ensuring iteration outputs are properly extracted (so successful GraphQL iterations are not miscounted as failures), and documents the related execution-status redesign / SDK migration notes.
Changes:
- Add GraphQL output handling to loop-iteration result extraction (
extractResultData) and related debug typing. - Add a regression test validating Loop + GraphQL runner returns non-nil per-iteration results and makes the expected HTTP calls.
- Extend execution-status redesign documentation with SDK migration/back-compat notes for the retired legacy enum value.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| docs/changes/0001-execution-status-redesign.md | Documents ava-sdk-js enum updates and legacy PARTIAL_SUCCESS (proto value 4) mapping behavior. |
| core/taskengine/vm_runner_graphql_query_test.go | Adds regression coverage for Loop + GraphQL runner iteration result extraction. |
| core/taskengine/vm.go | Handles step.GetGraphql() in loop iteration result extraction and node-type logging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…tus redesign Documents the full decision tree for how Studio interprets execution status both with the current SDK (PartialSuccess workaround) and the target state after the SDK upgrade. Includes per-component mapping of where status logic lives and a migration checklist.
…to logging - Remove unused request body decode in test mock handler - Add missing ethTransfer case to loop iteration debug logging switch
Summary
extractResultDatawas missing thestep.GetGraphql()case, causing all GraphQL loop iterations to be counted as "failed" even though HTTP requests succeededTest plan
TestLoopWithGraphQLRunner— regression test for bug: Loop node GraphQL runner never executes during iterations #523 verifying GraphQL loop iterations return non-nil resultsTestGraphlQlNodeSimpleQuery— existing GraphQL test still passesmake test)🤖 Generated with Claude Code